Skip to content

Bring back async_cmd_check to check for parallel extension install completion and fix dry-run mode with parallel extensions - #5223

Open
Flamefire wants to merge 4 commits into
easybuilders:developfrom
Flamefire:parallel-ext-install-fix
Open

Bring back async_cmd_check to check for parallel extension install completion and fix dry-run mode with parallel extensions#5223
Flamefire wants to merge 4 commits into
easybuilders:developfrom
Flamefire:parallel-ext-install-fix

Conversation

@Flamefire

Copy link
Copy Markdown
Contributor

We currently have this documentation

and check whether the command has completed via the async_cmd_check method.

It is used by rpackage which calls into the super method:
https://github.com/easybuilders/easybuild-easyblocks/blob/e53ee56b0ddce722fb4b37371096b0eabae6cf6f/easybuild/easyblocks/generic/rpackage.py#L335

However it was removed by 36febd8 so the rpackage would crash.

This PR brings it back working together with this callback and the expectations:

  • When done set self.async_cmd_output
  • return self.async_cmd_task.done()

This allows rpackage to work again and makes the implementation match the documentation.

Additionally I found that --extended-dry-run would crash:

                    if self.dry_run or ext.async_cmd_task.done():
                        res = ext.async_cmd_task.result()

In the dry-run mode the task is never started.
I resolved that by using a dummy result

And finally dry-run did query the dependencies but then didn't use them yielding misleading output.
The fix is straight forward but changes indentation, so the diff looks larger than it is.

@Flamefire
Flamefire force-pushed the parallel-ext-install-fix branch from 1f45a6a to 62f6eb1 Compare July 30, 2026 12:02
@boegel boegel added the change label Aug 6, 2026
@boegel boegel added this to the release after release of Aug'26 milestone Aug 6, 2026
@boegel boegel changed the title Bring back async_cmd_check to check for parallel extension install completion and fix dry-run mode with parallel extensions Bring back async_cmd_check to check for parallel extension install completion and fix dry-run mode with parallel extensions Aug 6, 2026
@boegel

boegel commented Aug 26, 2026

Copy link
Copy Markdown
Member

We currently have this documentation

and check whether the command has completed via the async_cmd_check method.

It is used by rpackage which calls into the super method: https://github.com/easybuilders/easybuild-easyblocks/blob/e53ee56b0ddce722fb4b37371096b0eabae6cf6f/easybuild/easyblocks/generic/rpackage.py#L335

However it was removed by 36febd8 so the rpackage would crash.

It's true that async_cmd_check is still defined by the RPackage easyblock, but it serves no purpose anymore, I don't see how it could lead to a crash?

I don't see why we would reinstate async_cmd_check, which was removed when we switched to using run_shell_cmd for running shell commands asynchronously in:

So instead, I think we should:

  • remove the async_cmd_check method from RPackage easyblock;
  • update our to correctly reflect what's required in order to be able to install extensions in parallel;

@Flamefire

Copy link
Copy Markdown
Contributor Author

I don't see why we would reinstate async_cmd_check, which was removed when we switched to using run_shell_cmd for running shell commands asynchronously in:

The rpackage easyblock shows that this is a useful hook to check that the async command actually succeeded that simply checking the completion marker of the task doesn't allow: https://github.com/easybuilders/easybuild-easyblocks/blob/develop/easybuild/easyblocks/generic/rpackage.py#L335-L337

The base implementation of async_cmd_check does the simple: if not self.async_cmd_task.done():
But it also stores the results for use by the custom easyblocks, e.g. in the hook.

So I don't see this as a replacement but an enhancement and think we should keep it.
We could remove self.async_cmd_output as I guess custom easyblocks could get that via self.async_cmd_task.result() although it seems convenient

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants